Add new secret types to Applications.Core/secretstores #7816

lakshmimsft · 2024-08-16T18:58:33Z

Description

Add new types to Applications.Core/secretstores (basicAuthentication, azureWorkloadIdentity, awsIRSA)
Update convertor, tests.
Update existing ValidateAndMutateRequest() in /pkg/corerp/frontend/controller/secretstores/kubernetes.go
to check if required secret keys exist for current secret type. Add to existing unit tests.

Type of change

This pull request fixes a bug in Radius and has an approved issue (Recipe support for private Bicep registries #6917 ).

Fixes: Part of #6917

vishwahiremat · 2024-08-21T23:12:10Z

pkg/corerp/api/v20231001preview/secretstore_conversion.go

@@ -106,6 +106,12 @@ func toSecretStoreDataTypeDataModel(src *SecretStoreDataType) datamodel.SecretTy
 		return datamodel.SecretTypeGeneric


lets add validations here during conversion if the required keys are present for the particular type. ex: "clientID" and "tenantID" keys should be present for type "azureWorkloadIdentity" type

We discussed offline. Currently validations are included in

radius/pkg/corerp/frontend/controller/secretstores/kubernetes.go

Line 90 in e577624

func ValidateAndMutateRequest(ctx context.Context, newResource *datamodel.SecretStore, oldResource *datamodel.SecretStore, options *controller.Options) (rest.Response, error) {

in line with other validations required for creating this resource. This function is accessed by controller here:

radius/pkg/corerp/setup/setup.go

Line 172 in e577624

_ = ns.AddResource("secretStores", &builder.ResourceOption[*datamodel.SecretStore, datamodel.SecretStore]{

including @kachawla to confirm this is the right approach.

codecov · 2024-08-22T17:01:36Z

Codecov Report

Attention: Patch coverage is 36.84211% with 12 lines in your changes missing coverage. Please review.

Project coverage is 61.03%. Comparing base (84cb120) to head (896cfc1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...erp/api/v20231001preview/secretstore_conversion.go	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7816      +/-   ##
==========================================
- Coverage   61.08%   61.03%   -0.05%     
==========================================
  Files         523      523              
  Lines       27474    27493      +19     
==========================================
- Hits        16782    16781       -1     
- Misses       9210     9226      +16     
- Partials     1482     1486       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

radius-functional-tests · 2024-08-22T17:57:30Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	lakshmimsft/radius-mainfork
Commit ref	`e577624`
Unique ID	func0cf485eb0f
Image tag	pr-func0cf485eb0f

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func0cf485eb0f
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func0cf485eb0f
controller test image location: ghcr.io/radius-project/dev/controller:pr-func0cf485eb0f
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func0cf485eb0f
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

radius-functional-tests · 2024-08-28T00:01:17Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	lakshmimsft/radius-mainfork
Commit ref	`d3e04e8`
Unique ID	func165aa50b04
Image tag	pr-func165aa50b04

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func165aa50b04
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func165aa50b04
controller test image location: ghcr.io/radius-project/dev/controller:pr-func165aa50b04
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func165aa50b04
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded
❌ corerp-cloud functional test failed. Please check the logs for more details

vishwahiremat

lgtm

vishwahiremat · 2024-08-28T23:53:56Z

pkg/corerp/frontend/controller/secretstores/kubernetes_test.go

-		newResource := testutil.MustGetTestData[datamodel.SecretStore](testFileCertValueFrom)
-		newResource.Properties.Resource = ""
-		resp, err := ValidateAndMutateRequest(context.TODO(), newResource, oldResource, nil)
+	tests := []struct {


radius-functional-tests · 2024-08-29T17:32:09Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	lakshmimsft/radius-mainfork
Commit ref	`babef6d`
Unique ID	func2ed15fc8d8
Image tag	pr-func2ed15fc8d8

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-func2ed15fc8d8
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-func2ed15fc8d8
controller test image location: ghcr.io/radius-project/dev/controller:pr-func2ed15fc8d8
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-func2ed15fc8d8
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

ytimocin · 2024-08-29T19:08:16Z

pkg/corerp/datamodel/secretstore.go

+	// SecretTypeAzureWorkloadIdentity is the azureFederatedIdentity secret type.
+	SecretTypeAzureWorkloadIdentity SecretType = "azureWorkloadIdentity"


Suggested change

// SecretTypeAzureWorkloadIdentity is the azureFederatedIdentity secret type.

SecretTypeAzureWorkloadIdentity SecretType = "azureWorkloadIdentity"

// SecretTypeAzureWorkloadIdentity is the azureWorkloadIdentity secret type.

SecretTypeAzureWorkloadIdentity SecretType = "azureWorkloadIdentity"

ytimocin · 2024-08-29T19:09:34Z

pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_AWSIRSA.json

nit: pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_awsirsa.json

ytimocin · 2024-08-29T19:09:50Z

...erp/frontend/controller/secretstores/testdata/secretstores_datamodel_AZWorkloadIdentity.json

nit: pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_azwi.json

ytimocin · 2024-08-29T19:10:11Z

...rp/frontend/controller/secretstores/testdata/secretstores_datamodel_basicAuthentication.json

nit: pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_basicauth.json

ytimocin · 2024-08-29T19:10:34Z

...end/controller/secretstores/testdata/secretstores_datamodel_basicAuthentication_invalid.json

nit: pkg/corerp/frontend/controller/secretstores/testdata/secretstores_datamodel_basicauth_invalid.json

radius-functional-tests · 2024-08-29T20:53:35Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	lakshmimsft/radius-mainfork
Commit ref	`b5527f7`
Unique ID	funcaad93367ab
Image tag	pr-funcaad93367ab

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcaad93367ab
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcaad93367ab
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcaad93367ab
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcaad93367ab
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
✅ datastoresrp-cloud functional tests succeeded
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

radius-functional-tests · 2024-08-29T21:37:43Z

Radius functional test overview

🔍 Go to test action run

Name	Value
Repository	lakshmimsft/radius-mainfork
Commit ref	`896cfc1`
Unique ID	funcb7ea5236a7
Image tag	pr-funcb7ea5236a7

Click here to see the list of tools in the current test run

gotestsum 1.12.0
KinD: v0.20.0
Dapr: 1.12.0
Azure KeyVault CSI driver: 1.4.2
Azure Workload identity webhook: 1.3.0
Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcb7ea5236a7
Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcb7ea5236a7
controller test image location: ghcr.io/radius-project/dev/controller:pr-funcb7ea5236a7
ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcb7ea5236a7
deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting corerp-cloud functional tests...
⌛ Starting ucp-cloud functional tests...
⌛ Starting datastoresrp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ datastoresrp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

kachawla · 2024-08-29T23:13:58Z

typespec/Applications.Core/environments.tsp

@@ -79,7 +79,7 @@ model RecipeConfigProperties {
  @doc("Configuration for Terraform Recipes. Controls how Terraform plans and applies templates as part of Recipe deployment.")
  terraform?: TerraformConfigProperties;

-  @doc("Environment variables injected during Terraform Recipe execution for the recipes in the environment.")
+  @doc("Environment variables injected during recipe execution for the recipes in the environment.")


@lakshmimsft what's causing this to change?

Correcting the comment to indicate environment variables schema is part of RecipeConfigProperties and is not intended to be specific to Terraform execution.

@lakshmimsft it is specific to Terraform though at this point. Unless we are actually injecting these env vars for non-Terraform recipes, we shouldn't indicate it is. As a user, I'd then set it assuming it works for bicep as well, which it doesn't.

This was a question asked in a design meeting: radius-project/design-notes#53 (comment)
Perhaps the comment should be more clear, eg: Environment variables injected during recipe execution for the recipes in the environment, currently supported for Terraform recipes.
Updated in in #7867

kachawla · 2024-08-29T23:15:56Z

typespec/Applications.Core/secretstores.tsp

+  @doc("basicAuthentication type is used to represent username and password based authentication and the secretstore resource is expected to have the keys 'username' and 'password'.")
+  basicAuthentication,
+
+  @doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")


Do we need to call out "registry authentication" here? This resource is/can be referenced in multiple places with use cases being not limited to ACR. I'd also get PMs eyes on these user facing descriptions.

cc @vishwahiremat, @Reshrahim . referring to link

I agree with not specifying registry here as it can be referenced elsewhere as well.

Suggested change

@doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")

@doc("azureWorkloadIdentity type is used to represent authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")

Suggested change

@doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")

@doc("azureWorkloadIdentity type is used to represent authentication using azure workload identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")

I think it is referenced as Azure workload identity and not federated identity in the docs

Thanks @Reshrahim. What are your thoughts on explicitly mentioned "registry" authentication? Secret store resource is not limited to supporting management of secrets only for the use case of registries.

Yeah sorry din notice the comment above. I agree with your comment of calling it as authentication as the uses cases can be outside of registries

Thanks for confirming @Reshrahim.

@vishwahiremat @lakshmimsft - could one of you please update this as per Reshma's feedback?

kachawla · 2024-08-29T23:17:18Z

typespec/Applications.Core/secretstores.tsp

+  @doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")
+  azureWorkloadIdentity,
+
+  @doc("awsIRSA type is used to represent registry authentication using AWS IRSA(IAM Roles for Service accounts) and the secretstore resource is expected to have the keys 'roleARN'.")


Same as above.

Couple of nits (can't add suggestions since the PR is merged):

Missing space between IRSA and (

key instead of keys

kachawla · 2024-08-29T23:19:51Z

pkg/corerp/frontend/controller/secretstores/types.go

+	RequiredUsername = "username"
+	RequiredPassword = "password"
+	RequiredClientId = "clientId"
+	RequiredTenantId = "tenantId"
+	RequiredRoleARN  = "roleARN"


The prefix Required is confusing, maybe I'm not following the thought process? They are the expected keys in the resource, so in general for such variables the convention is to add a key suffix, <key_name>Key, so it would be UsernameKey for example.

updated in in #7867

kachawla · 2024-08-29T23:22:40Z

pkg/corerp/frontend/controller/secretstores/types.go

@@ -19,4 +19,11 @@ package secretstores
 const (
 	// ResourceTypeName is the resource type name for secret stores.
 	ResourceTypeName = "Applications.Core/secretStores"
+
+	// The following are possible required keys in a SecretStore depending on it's SecretType


nit: let's not use different comment formats within the same file (example above for ResourceTypeName).

https://tip.golang.org/doc/comment#var

kachawla · 2024-08-29T23:34:35Z

pkg/corerp/frontend/controller/secretstores/kubernetes.go

@@ -75,8 +78,15 @@ func getOrDefaultEncoding(t datamodel.SecretType, e datamodel.SecretValueEncodin
 	return e, err
 }

+// Define a map of required keys for each SecretType
+var requiredKeys = map[datamodel.SecretType][]string{


This seems to be only referenced from ValidateAndMutateRequest function so I'd assume it should be scoped to that. Could you tell me more about the motivation to define it here?

moved into function: #7867

kachawla · 2024-08-29T23:44:39Z

pkg/corerp/api/v20231001preview/secretstore_conversion.go

+	case datamodel.SecretTypeBasicAuthentication:
+		return to.Ptr(SecretStoreDataTypeBasicAuthentication)
+	case datamodel.SecretTypeAzureWorkloadIdentity:
+		return to.Ptr(SecretStoreDataTypeAzureWorkloadIdentity)
+	case datamodel.SecretTypeAWSIRSA:
+		return to.Ptr(SecretStoreDataTypeAwsIRSA)


Should a unit test be added?

added in #7867

# Description Updates per comments in #7816 ## Type of change - This pull request adds or changes features of Radius and has an approved issue (#6917 ). Fixes: Part of #6917

lakshmimsft force-pushed the lakshmimsft/secretStoresType branch from 4a6058a to 5f3023b Compare August 16, 2024 19:01

vishwahiremat reviewed Aug 21, 2024

View reviewed changes

lakshmimsft force-pushed the lakshmimsft/secretStoresType branch from d1437c4 to e577624 Compare August 22, 2024 16:49

lakshmimsft marked this pull request as ready for review August 22, 2024 16:51

lakshmimsft requested review from a team as code owners August 22, 2024 16:51

lakshmimsft temporarily deployed to functional-tests August 22, 2024 17:56 — with GitHub Actions Inactive

lakshmimsft force-pushed the lakshmimsft/secretStoresType branch from e577624 to d3e04e8 Compare August 27, 2024 23:56

lakshmimsft temporarily deployed to functional-tests August 28, 2024 00:00 — with GitHub Actions Inactive

vishwahiremat previously approved these changes Aug 28, 2024

View reviewed changes

lakshmimsft force-pushed the lakshmimsft/secretStoresType branch from d3e04e8 to babef6d Compare August 29, 2024 17:30

lakshmimsft temporarily deployed to functional-tests August 29, 2024 17:31 — with GitHub Actions Inactive

ytimocin requested changes Aug 29, 2024

View reviewed changes

lakshmimsft added 6 commits August 29, 2024 12:29

upd to secretstore type

1c8c832

format tsp

ef474c3

updating tests to be table driven

d77161f

correcting comment on recipe env variables

8fc7505

run make generate

81fbfbe

updated per comments

b5527f7

lakshmimsft dismissed vishwahiremat’s stale review via b5527f7 August 29, 2024 20:13

lakshmimsft force-pushed the lakshmimsft/secretStoresType branch from babef6d to b5527f7 Compare August 29, 2024 20:13

lakshmimsft temporarily deployed to functional-tests August 29, 2024 20:52 — with GitHub Actions Inactive

ytimocin previously approved these changes Aug 29, 2024

View reviewed changes

lakshmimsft added 2 commits August 29, 2024 14:33

Rename file to temporary name to register case change

09f8a82

Rename file to lowercase name

896cfc1

lakshmimsft dismissed ytimocin’s stale review via 896cfc1 August 29, 2024 21:35

lakshmimsft temporarily deployed to functional-tests August 29, 2024 21:36 — with GitHub Actions Inactive

ytimocin approved these changes Aug 29, 2024

View reviewed changes

lakshmimsft merged commit 50fef93 into radius-project:main Aug 29, 2024
29 checks passed

kachawla reviewed Aug 29, 2024

View reviewed changes

lakshmimsft mentioned this pull request Aug 30, 2024

Updates to adding secret types PR #7867

Merged

willtsai mentioned this pull request Oct 23, 2024

Feature specs: Applications.Core/secretStores extended use cases radius-project/design-notes#57

Merged

		@@ -106,6 +106,12 @@ func toSecretStoreDataTypeDataModel(src *SecretStoreDataType) datamodel.SecretTy
		return datamodel.SecretTypeGeneric

		// SecretTypeAzureWorkloadIdentity is the azureFederatedIdentity secret type.
		SecretTypeAzureWorkloadIdentity SecretType = "azureWorkloadIdentity"

	@doc("azureWorkloadIdentity type is used to represent registry authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")
	@doc("azureWorkloadIdentity type is used to represent authentication using azure federated identity and the secretstore resource is expected to have the keys 'clientId' and 'tenantId'.")

Add new secret types to Applications.Core/secretstores #7816

Add new secret types to Applications.Core/secretstores #7816

Conversation

lakshmimsft commented Aug 16, 2024

Description

Type of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

radius-functional-tests bot commented Aug 22, 2024 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Aug 28, 2024 • edited Loading

Radius functional test overview

Test Status

vishwahiremat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radius-functional-tests bot commented Aug 29, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radius-functional-tests bot commented Aug 29, 2024 • edited Loading

Radius functional test overview

Test Status

radius-functional-tests bot commented Aug 29, 2024 • edited Loading

Radius functional test overview

Test Status

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kachawla Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

lakshmimsft Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reshrahim Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Reshrahim Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2024 •

edited

Loading

radius-functional-tests bot commented Aug 22, 2024 •

edited

Loading

radius-functional-tests bot commented Aug 28, 2024 •

edited

Loading

radius-functional-tests bot commented Aug 29, 2024 •

edited

Loading

radius-functional-tests bot commented Aug 29, 2024 •

edited

Loading

radius-functional-tests bot commented Aug 29, 2024 •

edited

Loading

kachawla Aug 29, 2024 •

edited

Loading

lakshmimsft Aug 30, 2024 •

edited

Loading

Reshrahim Sep 3, 2024 •

edited

Loading

Reshrahim Sep 3, 2024 •

edited

Loading